client/ssh/server/getent_test.go (1)

67-71: Minor: Redundant assertion after empty check.

Line 70's assertion len(shell) > 0 is redundant since line 67's condition already ensures shell != "" at this point.

♻️ Suggested simplification

 	shell := getShellFromGetent(current.Uid)
 	if shell == "" {
 		t.Log("getShellFromGetent returned empty, getent may not be available")
-	} else {
-		assert.True(t, len(shell) > 0, "shell should be non-empty when getent is available")
 	}
+	// If we reach here with non-empty shell, the test passed
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/server/getent_test.go` around lines 67 - 71, The test contains a
redundant assertion after checking for an empty shell: in getent_test.go the
if/else around getShellFromGetent() logs when shell == "" and then asserts
len(shell) > 0 in the else branch, which is unnecessary; simplify by removing
the redundant assert in the else branch (or replace the else with a single
assert that fails with a clear message when shell == ""), keeping reference to
getShellFromGetent and the test's logging/assertion logic.

client/ssh/server/getent_unix.go (1)

103-106: Returning nil, nil for empty output may cause confusion.

When id -G returns empty output, this returns (nil, nil). Callers may interpret this as "no groups" vs "error occurred". Consider returning an empty slice []string{} instead of nil to make the "no groups" case explicit.

♻️ Suggested improvement

 	trimmed := strings.TrimSpace(string(out))
 	if trimmed == "" {
-		return nil, nil
+		return []string{}, nil
 	}
 	return strings.Fields(trimmed), nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/server/getent_unix.go` around lines 103 - 106, The function that
handles the output of `id -G` currently returns (nil, nil) when `trimmed == ""`,
which is ambiguous; update the branch that checks `trimmed :=
strings.TrimSpace(string(out))` so it returns an explicit empty slice (e.g.
`[]string{}`) and nil error instead of `nil, nil`—locate the function that
invokes `id -G`/parses groups (the block using `trimmed` in getent_unix.go) and
change the return to `return []string{}, nil` to clearly signal “no groups.”

client/ssh/server/getent_unix_test.go (1)

228-235: TestRunGetent_NotAvailable is non-deterministic and likely always skipped in CI.

Consider moving command lookup/execution behind a small injectable function so this path can be unit-tested deterministically instead of depending on host tooling.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/server/getent_unix_test.go` around lines 228 - 235,
TestRunGetent_NotAvailable relies on the host having/ not having the "getent"
binary and is non-deterministic; refactor runGetent to accept or call an
injectable lookup/exec hook (e.g., replace direct exec.LookPath/exec.Command
usage with a package-level variable/function like lookupPath(name string)
(string, error) and commandRunner(name string, args ...string) *Cmd) so tests
can override them. Change runGetent to use these hooks (defaulting to
exec.LookPath and exec.Command) and update TestRunGetent_NotAvailable to
override lookupPath to return an error, asserting runGetent("root") fails
deterministically. Ensure the hooks are only used in runGetent and exposed for
tests without changing public API semantics.

client/ssh/server/getent_unix_test.go (1)

237-253: ⚠️ Potential issue | 🟡 Minor

Add id-binary availability guards to runIdGroups tests.

These tests directly execute id -G but don’t skip when id is missing, which makes them infra-dependent on minimal Unix environments.

Proposed fix

 func TestRunIdGroups_CurrentUser(t *testing.T) {
+	if _, err := exec.LookPath("id"); err != nil {
+		t.Skip("id not available on this system")
+	}
+
 	current, err := user.Current()
 	require.NoError(t, err)
@@
 func TestRunIdGroups_NonexistentUser(t *testing.T) {
+	if _, err := exec.LookPath("id"); err != nil {
+		t.Skip("id not available on this system")
+	}
+
 	_, err := runIdGroups("nonexistent_user_xyzzy_12345")
 	assert.Error(t, err)
 }
@@
 func TestIdGroupsMatchStdlib(t *testing.T) {
+	if _, err := exec.LookPath("id"); err != nil {
+		t.Skip("id not available on this system")
+	}
+
 	current, err := user.Current()
 	require.NoError(t, err)

Also applies to: 296-310

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/server/getent_unix_test.go` around lines 237 - 253, The tests
TestRunIdGroups_CurrentUser and TestRunIdGroups_NonexistentUser call the
external "id -G" binary and must be skipped if that binary is not available;
update both tests (and the similar tests at lines 296-310) to early-check
exec.LookPath("id") and call t.Skipf("skipping test: id binary not found") when
LookPath returns an error so the tests are not infra-dependent; keep the guard
at the top of each test before calling runIdGroups to ensure the skip happens
before any assertions.

client/ssh/server/getent_test.go (1)

71-72: ⚠️ Potential issue | 🟡 Minor

Relax strict absolute-shell assertions to match runtime fallback behavior.

These checks can be flaky because the runtime can fall back to $SHELL, which may be non-absolute in valid environments.

Proposed test adjustment

+import "os/exec"
@@
-	assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell)
+	if shell[0] != '/' {
+		_, err := exec.LookPath(shell)
+		assert.NoError(t, err, "shell should be absolute or resolvable in PATH, got %q", shell)
+	}

Also applies to: 124-125, 164-165

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/server/getent_test.go` around lines 71 - 72, The test currently
asserts the shell is an absolute path with assert.True(t, shell[0] == '/', ...),
which is flaky because runtime can legitimately fall back to a non-absolute
$SHELL; replace that strict check with a relaxed one using
path/filepath.IsAbs(shell) (or strings.HasPrefix(shell, "/")) OR simply assert
that shell is non-empty and valid for the runtime (e.g. assert.NotEmpty(t,
shell) and skip absolute-path enforcement). Apply the same change to the other
occurrences in this file corresponding to the same assertion pattern (the lines
around the current occurrence and the ones mentioned at 124-125 and 164-165) so
all absolute-path assertions are relaxed consistently.